Conversation
JSON doesn't actually allow these (despite the json module in Python happily serializing / parsing 'nan'), and they make assertEqual fail since they're not equal to anything, including themselves.
…json-patch into mithrandi-property-tests
stefankoegl
force-pushed
the
hypothesis
branch
from
1f4dc2a to
1d89ec3
Compare
There was a problem hiding this comment.
Pull Request Overview
This PR adds property-based testing using the Hypothesis framework to enhance test coverage for the jsonpatch library. The changes introduce comprehensive roundtrip testing that validates JSON patch generation and application across a wide range of automatically generated test cases.
- Adds Hypothesis as an optional development dependency for property-based testing
- Introduces roundtrip tests that verify patch generation and application work correctly together
- Refactors the DiffBuilder class to simplify its implementation and fix index tracking for list operations
Reviewed Changes
Copilot reviewed 4 out of 5 changed files in this pull request and generated 4 comments.
| File | Description |
|---|---|
| tests.py | Adds Hypothesis import handling, JSON strategy definition, and RoundtripTests class with property-based testing |
| requirements-dev.txt | Adds Hypothesis dependency with Python version constraints |
| jsonpatch.py | Major refactor of DiffBuilder class to simplify logic and improve index change tracking |
| .travis.yml | Moves development dependency installation to main install phase for CI |
| add_index_change('/', 1, +1) | ||
| """ | ||
|
|
||
| path_changes = self.index_changes.get(path, {}) |
There was a problem hiding this comment.
The path_changes dictionary is retrieved but never stored back to self.index_changes. This means the key_change calculation on line 778 has no effect. You need to store path_changes back: self.index_changes[path] = path_changes
| st.text(string.printable)]), | ||
| lambda children: | ||
| st.lists(children) | ||
| | st.dictionaries(st.text(string.printable), children)) |
There was a problem hiding this comment.
Using string.printable for JSON text generation can produce invalid JSON strings containing control characters like newlines and tabs that need escaping. Consider using st.text() without the alphabet parameter or st.text(alphabet=string.printable.replace('\n', '').replace('\t', '').replace('\r', ''))
| st.text(string.printable)]), | |
| lambda children: | |
| st.lists(children) | |
| | st.dictionaries(st.text(string.printable), children)) | |
| st.text(alphabet=string.printable.replace('\n', '').replace('\t', '').replace('\r', ''))]), | |
| lambda children: | |
| st.lists(children) | |
| | st.dictionaries(st.text(alphabet=string.printable.replace('\n', '').replace('\t', '').replace('\r', '')), children)) |
| storage[value] = [index] | ||
| else: | ||
| storage[value].append(index) | ||
| self.ops = [] |
There was a problem hiding this comment.
The index_changes attribute lacks documentation. Add a docstring or comment explaining its purpose: tracking cumulative index changes for list operations to maintain correct indices during patch generation.
| self.ops = [] | |
| self.ops = [] | |
| # Tracks cumulative index changes for list operations to maintain correct indices during patch generation. |
| self.add_index_change(path, key+1, -1) | ||
| self.add_index_change(path, newkey+1, +1) |
There was a problem hiding this comment.
When an item is moved, adjusting indices at key+1 position for the source doesn't account for the fact that the item is being moved, not just removed. This could lead to incorrect index calculations for subsequent operations.
| self.add_index_change(path, key+1, -1) | |
| self.add_index_change(path, newkey+1, +1) | |
| # Correctly adjust indices for items between key and newkey | |
| if key < newkey: | |
| # Moving forward: shift items between key+1 and newkey left by 1 | |
| for idx in range(key + 1, newkey + 1): | |
| self.add_index_change(path, idx, -1) | |
| elif key > newkey: | |
| # Moving backward: shift items between newkey and key-1 right by 1 | |
| for idx in range(newkey, key): | |
| self.add_index_change(path, idx, +1) |
2 participants
Continues the work from #63